New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for AWS authentication #347
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @focusaurus! A few minor comments, then one about supporting other body types.
app/network/network.js
Outdated
host: parsedUrl.hostname, | ||
headers: {} | ||
}; | ||
const body = req.body && req.body.text; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only works with "Text" body types like JSON/XML/etc. Other body types, like form application/x-www-form-urlencoded
, have a body object that looks different:
{
mimeType: 'application/x-www-form-urlencoded',
params: [{name: 'foo', value: 'bar'}]
}
The final body for these content-types is computed a few lines above, making it pretty easy to support application/x-www-form-urlencoded
. However, binary uploads and multipart/form-data
are handled by Curl so we don't ever know the final result of those. It would be possible to handle those other two cases ourselves but would take more work (maybe not worth it).
For the cases we don't support, it's probably worth throwing an error. The following code will throw an error for anything that isn't Text or application/x-www-form-urlencoded
.
const isForm = body.mimeType === 'multipart/form-data';
const isFile = body.fileName;
const isUnsupported = isForm || isFile;
if (isUnsupported) {
throw new Error('AWS authentication not supported for provided body type');
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For x-www-form-urlencoded if it's all just name/contents pairs I can convert that to a final string so aws4 can add it to the signature. But if there's file uploads I'm not sure how to get that to work properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the code for an exception for unsupported bodies. Here's my remaining questions:
- Do you want me to convert the x-www-form-urlencoded array of key/value objects which we pass to curl to a finalized and encoded string so it can be passed into aws4 to be signed?
- When running under
npm run dev
I can see the unsupported body exception is thrown but the UI doesn't seem to indicate this anywhere. - I'm seeing my entered auth values disappear as I navigate off the AWS tab. Is that expected? Is there something about the new code that's not right that's causing that?
app/network/network.js
Outdated
const parsedUrl = urlParse(url); | ||
const options = { | ||
path: parsedUrl.path, | ||
host: parsedUrl.hostname, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what AWS expects here, but should this be parsedUrl.host
instead of parsedUrl.hostname
? I think the only difference is that host
would include the port if it were specified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think parsedUrl.hostname
is correct. I tried explicitly including :443
in my URL and using host
but AWS gave me a 403 for that. Since in all normal cases AWS services are accessed via HTTPS using the default port, I think omitting the port is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 👍 Might be worth adding a comment beside it so someone doesn't ask the same question again?
app/network/network.js
Outdated
const options = { | ||
path: parsedUrl.path, | ||
host: parsedUrl.hostname, | ||
headers: {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we not be passing req.headers
into this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. We could, but first we'd need to convert them to a node format object instead of an array of name/value objects. aws4 only looks at a few key headers (Content-Type, Content-Length, Host) plus the AWS-specific ones.
const signature = aws4.sign(options, credentials); | ||
return Object.keys(signature.headers) | ||
// Don't use the inferred content type aws4 provides | ||
.filter((name) => name !== 'content-type') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this wouldn't be needed if we passed the content-type
header in the initial options above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think you're correct here. Refactoring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually without this we end up sending a duplicate Content-Type header and the signature won't verify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I think this is good now 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more comments @focusaurus. And, I noticed you hade some questions on an outdated diff (I can't reply anymore), so I'll answer them here.
Do you want me to convert the x-www-form-urlencoded array of key/value objects which we pass to curl to a finalized and encoded string so it can be passed into aws4 to be signed?
I need to do some refactoring with how the body building is done for the new plugin system, so I can handle this part later. For this PR, I'm fine with only handling text bodies.
When running under npm run dev I can see the unsupported body exception is thrown but the UI doesn't seem to indicate this anywhere.
I noticed this today as well while working on another branch. I pulled the fix into develop
so you can pull it into this branch.
I'm seeing my entered auth values disappear as I navigate off the AWS tab. Is that expected? Is there something about the new code that's not right that's causing that?
One of me comments below points out the cause of this. It was an easy one to miss.
app/models/request.js
Outdated
return { | ||
type, | ||
disabled: oldAuth.disabled || false, | ||
accessKeyId: oldAuth.username || '', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two lines should be:
accessKeyId: oldAuth.accesKeyId || '',
secretAccessKey: oldAuth.secretAccessKey || ''
const signature = aws4.sign(options, credentials); | ||
return Object.keys(signature.headers) | ||
// Don't use the inferred content type aws4 provides | ||
.filter((name) => name !== 'content-type') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I think this is good now 👍
} = this.props; | ||
|
||
const pairs = [{ | ||
name: authentication.username || '', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be
name: authentication.accesKeyId || '',
value: authentication.secretAccessKey || ''
This explains why you are seeing your values disappear from the UI.
* Implements Kong#344 * Multifactor authentication (MFA) not yet supported
3b99cec
to
a09d35d
Compare
OK thanks for the help. I've made your most recent batch of changes and rebased to clean up the noise. I think this might be ready to merge now. Still room to make the features more thorough in the future, but this at least allows me to call AWS API Gateway endpoints with text bodies, which otherwise is pretty much only possible programmatically as even curl and httpie make it exceedingly difficult. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Merging this in 😄
Thanks @focusaurus! Will try to get an update out by the end of the week with this included. |
Thanks @focusaurus this is awesome! |
What
Why
Allow end users to easily send requests to Amazon Web Services APIs. Although this authentication scheme is non-standard, the service is widely used and difficult to use without good support from developer tools.
Details
Follows closely existing patterns for other authentication schemes.
Notes
More work would be required to support Multifactor authentication, which requires an additional field AWS_SESSION_TOKEN. I can do that as well but would like your guidance on the UI.
Related Issue: #344